feat: add modal to create new assignment - #1396
Conversation
0d76a0f to
59c2395
Compare
59c2395 to
49a5f6a
Compare
|
|
||
| const peopleWorkload = calculatePeopleWorkload(clusters, queueItems) | ||
|
|
||
| openOverlayModal({ |
There was a problem hiding this comment.
await this?
It seems we have inconsistent patterns across the codebase for this so I'm not sure what the correct pattern is.
If it is awaited then the noop .catch(() => {}) is probably redundant, considering the following catch.
There was a problem hiding this comment.
Since it's in an async, awaiting would make sense to me. . .
| } else if (props.message.type === 'assign') { | ||
| const message = props.message | ||
| } else if (props.message.type === 'assign' || props.message.type === 'add') { | ||
| const rfcToBeId = props.message.rfcToBeId |
There was a problem hiding this comment.
nit
| const rfcToBeId = props.message.rfcToBeId | |
| const { rfcToBeId } = props.message |
holloway
left a comment
holloway
left a comment
There was a problem hiding this comment.
lgtm only minor feedback
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
Tying the modal's ability to select the role / hide the blocked role specifically to the add message feels awkward to me. I'd have expected that to be an explicit option to the modal instead of tying its functionality that tightly to the workflows we have at the moment.
Just a comment because I mostly defer to @holloway on the PR overall.
3 participants
fix #1334